Skip to content

Update esp32_ledc_mcu.cpp to ensure ledc_timer is initialised to a known state (ledc_timer = false) #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

rob-deutsch
Copy link
Contributor

@rob-deutsch rob-deutsch commented Jan 17, 2025

Need to set this uninitialised piece of memory, otherwise ledc_timer_config() might try to call ledc_timer_del() and that will fail.

@rob-deutsch rob-deutsch changed the title Update esp32_ledc_mcu.cpp to set ledc_timer = false Update esp32_ledc_mcu.cpp to set ledc_timer.deconfigure = false Jan 17, 2025
@askuric
Copy link
Member

askuric commented Jan 20, 2025

Thanks for this!
This seems to be a API change actually going from the espressif 5.1 to 5.2
Introducing the deconfigure might actually break the code for people with v3.0.x of the arduino-esp32 which depends on espressif 5.1.4. So we will have to handle this with care 😄

ledc_timer_config_t definition

Could you just confirm which version of espressif and ardunio-esp32 are you using?

@rob-deutsch
Copy link
Contributor Author

Oh good pickup. I'm actually using pioarduino because espressif/arduino-esp32 doesn't seem to support platformio any more.

Maybe the simplest way forward is to calloc/memset ledc_timer_config_t to 0 based on its size?

@rob-deutsch rob-deutsch changed the title Update esp32_ledc_mcu.cpp to set ledc_timer.deconfigure = false Update esp32_ledc_mcu.cpp to ensure ledc_timer is initialised to a known state (ledc_timer = false) Jan 28, 2025
@rob-deutsch
Copy link
Contributor Author

Any thoughts on why the tests are failing? I didn't change anything which should break those tests, and the error message isn't clear to me.

@runger1101001
Copy link
Member

Any thoughts on why the tests are failing? I didn't change anything which should break those tests, and the error message isn't clear to me.

It was unrelated to your change and I have fixed it.

Introducing the deconfigure might actually break the code for people with v3.0.x of the arduino-esp32 which depends on espressif 5.1.4. So we will have to handle this with care 😄

So how should we handle it? I must say I find the current situation with ESP32 very confusing. Some boards still work with the old framework versions, also in platformio. Other boards (C6, C5, P4) require the new framework. The new framework official release doesn't work in PlatformIO, but the new pioarduino framework does.

@runger1101001 runger1101001 added the bug Something isn't working label Feb 18, 2025
@rob-deutsch
Copy link
Contributor Author

Introducing the deconfigure might actually break the code for people with v3.0.x of the arduino-esp32 which depends on espressif 5.1.4. So we will have to handle this with care 😄

So how should we handle it?

Good question - the updated branch I forced push will work across any and all versions of arduino-esp32 and pioarduino. It's entirely cross platform because it just inits the struct to zeroes.

I would suggest merging this PR and tackling the question about ESP32 compatability elsewhere.

I must say I find the current situation with ESP32 very confusing. Some boards still work with the old framework versions, also in platformio. Other boards (C6, C5, P4) require the new framework. The new framework official release doesn't work in PlatformIO, but the new pioarduino framework does.

I also find the current situation confusing. It's an unfortunate situation that seems to be caused by Espressif no longer supporting platformio in arduino-esp32.

I don't yet have a strong POV as to what the right thing to do is. I'm sure that simplefoc isn't the only one facing this problem.

@runger1101001
Copy link
Member

Ok, I'm still a bit confused by this line of the diff:

image

But I will merge that and solve the #endif separately if there is an issue with it. It looks like it was both removed and added, weird.

@runger1101001 runger1101001 added this to the 2.3.5_Release milestone Feb 21, 2025
@runger1101001 runger1101001 merged commit fb0be07 into simplefoc:master Feb 21, 2025
21 of 22 checks passed
@rob-deutsch
Copy link
Contributor Author

I suspect that's an accidental addition of a linefeed (LF) character at the end of the line.

Would you like me to submit a PR which removes this additional LF character?

@runger1101001
Copy link
Member

No, thanks for offering this, but I have already merged it :-) I made the mistake of merging it to master, doh! That's what I actually should have checked, but never mind. Its really just a small change, and I'll fix all this in time for the next release...

@rob-deutsch
Copy link
Contributor Author

Sorry about that, I should've spotted it before submitting the PR. Thanks for your understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants